Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update survey proto to support data sharing agreement #1940

Merged
merged 10 commits into from
Aug 8, 2024

Conversation

nwkotto
Copy link
Contributor

@nwkotto nwkotto commented Aug 2, 2024

Part 1 for #1856

@nwkotto nwkotto requested a review from gino-m August 2, 2024 19:55
@sufyanAbbasi
Copy link
Contributor

Awesome! And I believe we will also want an associated response field on the Submission too, since we'll want to save the user's response to the DB in case the survey organizers change the sharing consent after the fact. Maybe just an enum will suffice?

@gino-m
Copy link
Collaborator

gino-m commented Aug 2, 2024

Awesome! And I believe we will also want an associated response field on the Submission too, since we'll want to save the user's response to the DB in case the survey organizers change the sharing consent after the fact. Maybe just an enum will suffice?

Not sure about storing Submission - a Firestore query would scan all submissions to find out if one has the consent bit. That said, Survey isn't editable.. maybe a new collection specifically for this purpose? For a first plass we'll just ask once per session per survey, so nothing needs to be stored.

@nwkotto
Copy link
Contributor Author

nwkotto commented Aug 5, 2024

Awesome! And I believe we will also want an associated response field on the Submission too, since we'll want to save the user's response to the DB in case the survey organizers change the sharing consent after the fact. Maybe just an enum will suffice?

Not sure about storing Submission - a Firestore query would scan all submissions to find out if one has the consent bit. That said, Survey isn't editable.. maybe a new collection specifically for this purpose? For a first plass we'll just ask once per session per survey, so nothing needs to be stored.

So are we saying we're ok to not include any submission bit about consent being agreed to? Or how are we expecting that to be captured?

Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits, thanks @nwkotto

proto/src/google/ground/v1beta1/survey.proto Outdated Show resolved Hide resolved
proto/src/google/ground/v1beta1/survey.proto Outdated Show resolved Hide resolved
proto/src/google/ground/v1beta1/survey.proto Outdated Show resolved Hide resolved
@gino-m
Copy link
Collaborator

gino-m commented Aug 6, 2024

Awesome! And I believe we will also want an associated response field on the Submission too, since we'll want to save the user's response to the DB in case the survey organizers change the sharing consent after the fact. Maybe just an enum will suffice?

Not sure about storing Submission - a Firestore query would scan all submissions to find out if one has the consent bit. That said, Survey isn't editable.. maybe a new collection specifically for this purpose? For a first plass we'll just ask once per session per survey, so nothing needs to be stored.

So are we saying we're ok to not include any submission bit about consent being agreed to? Or how are we expecting that to be captured?

Sorry, I think I misunderstood the original question. Adding a bit to submissions as to which consent was given is a good idea. For now the Android client will need to store that consent was given on the first data collection flow locally, in the future we may store that in the remote db as well.

@gino-m gino-m changed the title Made survey proto updates to support data sharing agreement Update survey proto to support data sharing agreement Aug 6, 2024
@nwkotto nwkotto requested a review from gino-m August 7, 2024 21:45
Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a few small adjustments, LGTM!

@nwkotto nwkotto merged commit a3f5e28 into master Aug 8, 2024
4 checks passed
@nwkotto nwkotto deleted the data-sharing-protos branch August 8, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants